-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Improvements for "StringValuesBuilder#appendAll" #4870
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Improvements for "StringValuesBuilder#appendAll" #4870
Conversation
WalkthroughThe Changes
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, could you please run ./gradlew apiDump
and commit the result to update the API snapshot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @otobrglez, do you want to continue work on this PR?
We're preparing release 3.2.0 this week so it is the best time to include new APIs.
Yeah, I would love to. Please guide me. 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ktor-utils/common/test/io/ktor/util/StringValuesTest.kt (1)
120-135
: Enhance test assertions for comprehensive validation.The test covers all four new
appendAll
extension functions, which is excellent. However, the assertions could be more thorough to ensure all values are correctly stored:Consider enhancing the assertions to verify all values:
assertTrue { map.contains("foo") } assertTrue { map.contains("foo", "bar") } assertTrue { map.contains("baz") } + assertTrue { map.contains("baz", "bar") } + assertTrue { map.contains("baz", "bor") } + assertEquals(listOf("bar", "bor"), map.getAll("baz")) assertTrue { map.contains("rar") } + assertTrue { map.contains("rar", "boo") } + assertTrue { map.contains("rar", "too") } + assertEquals(listOf("boo", "too"), map.getAll("rar")) assertTrue { map.contains("goo", "boo") } assertTrue { map.contains("doo", "dah") }This follows the pattern established by other tests in the file (like
twoValueMap
andthreeValueCaseInsensitiveMap
) that use bothcontains()
andgetAll()
to verify multi-value scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ktor-utils/common/src/io/ktor/util/StringValues.kt
(3 hunks)ktor-utils/common/test/io/ktor/util/StringValuesTest.kt
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ktor-utils/common/src/io/ktor/util/StringValues.kt
🧰 Additional context used
🧬 Code Graph Analysis (1)
ktor-utils/common/test/io/ktor/util/StringValuesTest.kt (1)
ktor-utils/common/src/io/ktor/util/StringValues.kt (9)
appendAll
(117-117)appendAll
(118-118)appendAll
(258-262)appendAll
(270-275)appendAll
(441-445)appendAll
(475-477)appendAll
(487-490)appendAll
(500-503)appendAll
(513-516)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ktor-http/api/ktor-http.api
(1 hunks)ktor-utils/api/ktor-utils.api
(2 hunks)ktor-utils/api/ktor-utils.klib.api
(1 hunks)ktor-utils/common/test/io/ktor/util/StringValuesTest.kt
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ktor-utils/common/test/io/ktor/util/StringValuesTest.kt
🔇 Additional comments (3)
ktor-utils/api/ktor-utils.api (1)
395-398
: LGTM! Core functionality aligns with PR objectives.The addition of default implementations for
appendAll
methods withMap
parameters supports the stated goal of enabling bulk parameter appending from maps.ktor-utils/api/ktor-utils.klib.api (1)
943-946
: LGTM! Excellent ergonomic improvements to StringValuesBuilder.These four new
appendAll
extension functions provide comprehensive support for bulk appending operations from various collection types. The implementation correctly:
- Supports both
Map<String, String>
andMap<String, Iterable<String>>
for different use cases- Provides vararg overloads for
Pair<String, String>
andPair<String, Iterable<String>>
- Returns
StringValuesBuilder
for method chaining- Follows consistent naming conventions with existing
appendAll
methodsThis enhancement will significantly improve the developer experience when building URL parameters from maps, exactly as described in the PR objectives.
ktor-http/api/ktor-http.api (1)
831-834
:✅ Verification successful
Verify method signatures for appendAll overloads.
Both
appendAll
andappendAllIterable
methods have identical JVM signatures(Lio/ktor/http/ParametersBuilder;Ljava/util/Map;)V
, which raises concerns about method resolution and API design.According to the PR objectives, the intent is to support:
Map<String, String>
Map<String, Iterable<String>>
However, due to type erasure, both would have the same erased signature. Please verify:
Method Differentiation: How are these methods distinguished at runtime when both accept
Map
with the same erased signature?Naming Convention: The
appendAllIterable
name seems verbose compared to standard Kotlin overloading patterns. Is this compiler-generated name mangling, or was this intentionally chosen?API Consistency: Ensure this aligns with the corresponding
StringValuesBuilder$DefaultImpls
mentioned in the AI summary.Let me verify the actual source code to understand how these methods are differentiated:
🏁 Script executed:
#!/bin/bash # Search for ParametersBuilder appendAll implementations to understand the actual method signatures ast-grep --pattern 'fun appendAll($$$) { $$$ }' # Also search for any StringValuesBuilder DefaultImpls for comparison rg -A 5 "DefaultImpls" # Look for the actual extension function declarations ast-grep --pattern 'fun ParametersBuilder.appendAll($$$) { $$$ }'Length of output: 28971
DefaultImpls methods are correctly differentiated
- The Kotlin compiler emits two distinct static methods—
appendAll
andappendAllIterable
—to prevent JVM signature clashes after type erasure.- This naming pattern exactly mirrors the implementation in
StringValuesBuilder$DefaultImpls
, ensuring consistency across the API.No changes required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the quick fix! Well done! I have only a few minor comments.
To keep your laptop in the room, try ./gradlew :ktor-utils:apiDump
🙂
* @param values the map containing key-value pairs to append | ||
* @return this builder instance | ||
*/ | ||
@JvmName("appendAllIterableWithMapList") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this annotation could be dropped
* @param values the map containing key-value pairs to append where values are [Iterable] of strings | ||
* @return this builder instance | ||
*/ | ||
@JvmName("appendAllIterableWithMap") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 The name can be the same as for vararg function - appendAllIterable
as argument types are different and signatures won't clash
public final class io/ktor/http/ParametersBuilder$DefaultImpls { | ||
public static fun appendAll (Lio/ktor/http/ParametersBuilder;Ljava/util/Map;)V | ||
public static fun appendAllIterable (Lio/ktor/http/ParametersBuilder;Ljava/util/Map;)V | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... BCV has included some extra lines from previous changes. Seems like a bug. Could you remove these lines and some extra lines in ktor-utils.api
?
Hey!
While using Ktor, I found that it could be more ergonomic if
StringValuesBuilder
also supported mutations with maps. This is my humble improvement that could make this experience better.Subsystem
ktor-utils
Motivation
I wanted to build URLs with something like this:
Solution
appendAll
toStringValuesBuilder
insideStringValues.kt
Map<String, String>
and another forMap<String, Iterable<String>>
append
andappendAll
)P.S.: Please be gentle, as this is my first PR to Ktor.
Regards
- Oto